-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Terraform removal from infrastructure reconciler #596
Conversation
@elchead Thank you for your contribution. |
@elchead You need rebase this pull request with latest master branch. Please check. |
Thank you @elchead for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" | ||
) | ||
|
||
// MakeCluster returns a cluster object used for testing. | ||
func MakeCluster(pods, services string, region string, countFaultDomain, countUpdateDomain int32) *controller.Cluster { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not group testing code with production code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree its suboptimal, but its an internal package. Should i create an entirely new test package that can be shared across different pkg tests?
E.g. pkg/internal/infrastructure/test
?
// TODO copy from AWS PR (use taskBuilder component to share?) | ||
// addTask adds a wrapped task for the given task function and options. | ||
func (f FlowReconciler) addTask(g *flow.Graph, name string, fn flow.TaskFn, options ...shared.TaskOption) flow.TaskIDer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use the "shared" library version for that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Martin's flowContext has a persistor which I don't need and I cannot opt out. Since it's only very few code and since the libraries are not synced at the moment at any rate, I would defer the refactor to opt out of the persistor until the libraries are really shared
/test |
Testrun: e2e-q88m8 +---------------------+---------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+---------------------+-----------+----------+ | infrastructure-test | infrastructure-test | Succeeded | 33m38s | | bastion-test | bastion-test | Succeeded | 11m3s | +---------------------+---------------------+-----------+----------+ |
/reviewed ok-to-test |
/test |
Testrun: e2e-b4fz7 +--------------------------+--------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +--------------------------+--------------------------+-----------+----------+ | infrastructure-test | infrastructure-test | Succeeded | 35m6s | | infrastructure-test-flow | infrastructure-test-flow | Failed | 5m20s | | bastion-test | bastion-test | Succeeded | 10m30s | +--------------------------+--------------------------+-----------+----------+ |
/test |
Testrun: e2e-zcmzw +--------------------------+--------------------------+--------+----------+ | NAME | STEP | PHASE | DURATION | +--------------------------+--------------------------+--------+----------+ | infrastructure-test | infrastructure-test | Failed | 42m35s | | infrastructure-test-flow | infrastructure-test-flow | Failed | 1h5m53s | | bastion-test | bastion-test | Failed | 12m27s | +--------------------------+--------------------------+--------+----------+ |
d50207a
to
5a7f047
Compare
@kon-angelo can you mention the PR when the feature gets merged? |
How to categorize this PR?
/area control-plane
/kind enhancement
/platform azure
What this PR does / why we need it:
Reconciliation of infrastructure resources are managed directly with a graph flow and using Azure SDK client API instead of using Terraformer.
Which issue(s) this PR fixes:
Fixes #411
Special notes for your reviewer:
Release note: